Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Use StorageNMap for Approvals in assets pallet #8816

Merged
5 commits merged into from
May 17, 2021
Merged

Use StorageNMap for Approvals in assets pallet #8816

5 commits merged into from
May 17, 2021

Conversation

KiChjang
Copy link
Contributor

Fixes #8291.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label May 14, 2021
@KiChjang KiChjang added the C1-low PR touches the given topic and has a low impact on builders. label May 14, 2021
@shawntabrizi shawntabrizi added the D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. label May 14, 2021
shawntabrizi
shawntabrizi approved these changes May 14, 2021
View reviewed changes
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks right to me :)

need to make sure @jacogr signs off on this before we actually merge

@jacogr jacogr mentioned this pull request May 15, 2021
Closed
@jacogr
Copy link
Contributor

jacogr commented May 15, 2021 •
edited
Loading

I have holes in the JS API atm - so stuff like the .keys and .entries iterators have not been done for NMap yet. (Generally prefer to have something in-storage first to be able to generate proper test-cases for e2e and actual offline/static, this provides that. Additionally, also thinking, as you are, of just ripping out stuff and making Map/DoubleMap/NMap use all the same stuff, e.g. combining underneath the hood... but that is most-probably a follow-up)

In this case it is purely the approvals for which I don't yet have something UI-wise. Additionally know of nothing out there tool, UI-wise or otherwise that uses this functionality atm. So it is technically non-breaking for existing tools. Even explorers need work anyway before they are able to index chains using this (e.g. the Python libs and Ruby libs), but since it is not on an existing chain, all is safe.

Need to get the first items sorted anyway API-wise before being able to do anything. (It is "just work", nicely marked, so no crisis - just need a gap)

TL:DR Get it in.

@shawntabrizi
Copy link
Member

bot merge

@ghost
Copy link

ghost commented May 17, 2021

Trying merge.

@ghost ghost merged commit 66c85ae into master May 17, 2021
@ghost ghost deleted the kckyeung/use-nmap-in-assets branch May 17, 2021 06:27
nazar-pc pushed a commit to autonomys/substrate that referenced this pull request Aug 8, 2021
@KiChjang @nazar-pc
Use StorageNMap for Approvals in assets pallet (paritytech#8816)
8e182f8 
* Use StorageNMap for Approvals in assets pallet

* Use EncodeLike on HashKeyPrefix trait bounds

* Add comments clarifying AccountId roles

* Properly document the keys in the Approvals storage

* Fix line width
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gavofyork gavofyork gavofyork approved these changes

@jacogr jacogr jacogr approved these changes

@shawntabrizi shawntabrizi shawntabrizi approved these changes

@gui1117 gui1117 Awaiting requested review from gui1117

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement StorageTripleMap
4 participants
@KiChjang @jacogr @shawntabrizi @gavofyork